-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#12596 ] Instructor extending individual deadlines: table doesn't sort by team… #12707
[#12596 ] Instructor extending individual deadlines: table doesn't sort by team… #12707
Conversation
Hi @suneelval, thank you for your interest in contributing to TEAMMATES!
Please address the above before we proceed to review your PR. |
Hey @suneelval, can you take a look at the failing E2E test? It's the one at |
@weiquu Test failure issue fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code change LGTM! just a query about the E2E test
src/e2e/resources/data/InstructorSessionIndividualExtensionPageE2ETest.json
Outdated
Show resolved
Hide resolved
@cedricongjh it seems in master branch some of Axe Test cases are failing, due to which build checks are failing kindly look into it, Thank you., |
thanks for getting back to us regarding the test data, as for the tests, we'll be looking into it |
"notifications": { | ||
"notification1": { | ||
"notificationId": "notification1", | ||
"startTime": "2011-01-01T00:00:00Z", | ||
"endTime": "2099-01-01T00:00:00Z", | ||
"createdAt": "2011-01-01T00:00:00Z", | ||
"style": "DANGER", | ||
"targetUser": "GENERAL", | ||
"title": "E2E notif for general users", | ||
"message": "<p>This notification is shown to general users</p>", | ||
"shown": false | ||
}, | ||
"notification2": { | ||
"notificationId": "notification2", | ||
"startTime": "2011-02-02T00:00:00Z", | ||
"endTime": "2099-02-02T00:00:00Z", | ||
"createdAt": "2011-01-01T00:00:00Z", | ||
"style": "SUCCESS", | ||
"targetUser": "STUDENT", | ||
"title": "E2E notif for students", | ||
"message": "<p>This notification is shown to students only</p>", | ||
"shown": false | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think these notifications are not needed in this e2e test case as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cedricongjh all review comments addressed, please review it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @suneelval, seems like the notifications are still there, could you remove them? also i have some other queries (do look at the other comments)
removed unwanted mock data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies for the late review, do address the 2 comments and it should be good to merge!
"notifications": { | ||
"notification1": { | ||
"notificationId": "notification1", | ||
"startTime": "2011-01-01T00:00:00Z", | ||
"endTime": "2099-01-01T00:00:00Z", | ||
"createdAt": "2011-01-01T00:00:00Z", | ||
"style": "DANGER", | ||
"targetUser": "GENERAL", | ||
"title": "E2E notif for general users", | ||
"message": "<p>This notification is shown to general users</p>", | ||
"shown": false | ||
}, | ||
"notification2": { | ||
"notificationId": "notification2", | ||
"startTime": "2011-02-02T00:00:00Z", | ||
"endTime": "2099-02-02T00:00:00Z", | ||
"createdAt": "2011-01-01T00:00:00Z", | ||
"style": "SUCCESS", | ||
"targetUser": "STUDENT", | ||
"title": "E2E notif for students", | ||
"message": "<p>This notification is shown to students only</p>", | ||
"shown": false | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @suneelval, seems like the notifications are still there, could you remove them? also i have some other queries (do look at the other comments)
"giver": "[email protected]", | ||
"recipient": "[email protected]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could I check why the emails were changed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cedricongjh
Comment1: notifications tag has been removed on Mar 26th here is the commit eba76ea
Comment2: "ISesIe." has been removed from giver and recipient email earlier emails looks like below
"giver": "**ISesIe.**[email protected]",
"recipient": "**ISesIe.**[email protected]",
changed to
"giver": "[email protected]",
"recipient": "[email protected]",
incorrect email is causing test case failure and updated with right email.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cedricongjh all issues has been addressed and build is stable please review it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suneelval Hey you shouldnt have to change the email to make it work maybe there is some underlying issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mingyuanc email Id changes has been reverted and build is table please review.
…://github.com/suneelval/teammates into Instructor_extending_table_sort_by_team_issue
E2E-testing (firefox, unstable) test failure issue fix
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
email id's changes reverted as per PR reviewer comments.
@@ -58,7 +58,7 @@ public void testAll() { | |||
|
|||
______TS("verify extend some deadlines, notifyUsers enabled"); | |||
|
|||
individualExtensionPage.selectStudents(0, 2); // alice and charlie | |||
individualExtensionPage.selectStudents(0, 1); // alice and charlie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for missing this out but any reason u changed this to 1?
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
@suneelval any updates? |
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
@suneelval closing due to inactivity please reopen when ready |
… issue fixed
Fixes #12596
Students table Sort by Team column typo fixed to sort correctly.
Outline of Solution
Typo error fixed.